Skip to content

Conversation

@folkertdev
Copy link
Contributor

@folkertdev folkertdev commented Dec 27, 2025

tracking issue: #44930

Implement va_copy as (the rust equivalent of) memcpy, which is the behavior of all current LLVM targets. By providing our own implementation, we can guarantee its behavior. These guarantees are important for implementing c-variadics in e.g. const-eval.

Discussed in #t-compiler/const-eval > c-variadics in const-eval.

I've also updated the comment for Drop a bit. The background here is that the C standard requires that va_end is used in the same function (and really, in the same scope) as the corresponding va_start or va_copy. That is because historically va_start would start a scope, which va_end would then close. e.g.

https://softwarepreservation.computerhistory.org/c_plus_plus/cfront/release_3.0.3/source/incl-master/proto-headers/stdarg.sol

#define         va_start(ap, parmN)     {\
        va_buf  _va;\
        _vastart(ap = (va_list)_va, (char *)&parmN + sizeof parmN)
#define         va_end(ap)      }
#define         va_arg(ap, mode)        *((mode *)_vaarg(ap, sizeof (mode)))

The C standard still has to consider such implementations, but for Rust they are irrelevant. Hence we can use Clone for va_copy and Drop for va_end.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 27, 2025
Comment on lines 158 to 182
/// Basic implementation of a `va_list`.
#[repr(transparent)]
#[derive(Debug)]
#[derive(Debug, Clone, Copy)]
struct VaListInner {
ptr: *const c_void,
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// https://github.com/llvm/llvm-project/blob/0cdc1b6dd4a870fc41d4b15ad97e0001882aba58/clang/lib/CodeGen/Targets/Hexagon.cpp#L407-L417
#[repr(C)]
#[derive(Debug)]
#[derive(Debug, Clone, Copy)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// https://github.com/llvm/llvm-project/blob/af9a4263a1a209953a1d339ef781a954e31268ff/llvm/lib/Target/Xtensa/XtensaISelLowering.cpp#L1211-L1215
#[repr(C)]
#[derive(Debug)]
#[derive(Debug, Clone, Copy)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// https://docs.google.com/gview?embedded=true&url=https://github.com/IBM/s390x-abi/releases/download/v1.7/lzsabi_s390x.pdf
#[repr(C)]
#[derive(Debug)]
#[derive(Debug, Clone, Copy)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// [GCC header]: https://web.mit.edu/darwin/src/modules/gcc/gcc/ginclude/va-ppc.h
#[repr(C)]
#[derive(Debug)]
#[derive(Debug, Clone, Copy)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@folkertdev folkertdev added the F-c_variadic `#![feature(c_variadic)]` label Dec 27, 2025
@rust-log-analyzer

This comment has been minimized.

@folkertdev folkertdev force-pushed the va-list-copy branch 3 times, most recently from 7a78640 to 8e09112 Compare January 2, 2026 21:02
@rustbot rustbot added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jan 2, 2026
@folkertdev
Copy link
Contributor Author

r? @workingjubilee

The codegen of VaList::clone is target-specific now, and because minicore does not define VaList it is hard to test. I think it's fine to just test the behavior, and have LLVM use a memcpy when VaList is a struct, or just re-use the pointer value if it's just a pointer.

@folkertdev folkertdev marked this pull request as ready for review January 2, 2026 21:04
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2026

workingjubilee is currently at their maximum review capacity.
They may take a while to respond.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2026
@rustbot
Copy link
Collaborator

rustbot commented Jan 2, 2026

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 2, 2026
@RalfJung
Copy link
Member

RalfJung commented Jan 3, 2026

Implement va_copy as (the rust equivalent of) memcpy, which is the behavior of all current LLVM targets. By providing our own implementation, we can guarantee its behavior.

Didn't we say we would avoid that assumption and also support implementations that do malloc/free in va_copy / va_end? See #t-compiler/const-eval > c-variadics in const-eval @ 💬.

@folkertdev
Copy link
Contributor Author

That's right, we don't actually impl Copy for VaList, so that a future target could implement a meaningful Clone and Drop. It's just that in practice for all current targets Clone happens to be a memcpy and Drop a no-op

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offering a suggested historical note.

Please feel free to trim it or move it into the PR description itself, down to the amounts you find worthwhile or just amusing to keep (including 0 lines).

r=me after

Thanks!

View changes since this review

@workingjubilee workingjubilee added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 6, 2026
@RalfJung
Copy link
Member

RalfJung commented Jan 7, 2026

@folkertdev what is your plan for va_copy regarding the const-eval support for this?
ISTM that without the intrinsic, it'll not be possible to catch a bunch of the UB that we had planned to catch?

@folkertdev folkertdev changed the title c_variadic: use Clone instead of va_copy c_variadic: use Clone instead of LLVM va_copy Jan 7, 2026
@rustbot

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

Based on further discussion in #t-compiler/const-eval > c-variadics in const-eval, a slight change of plans

  • we still no longer call LLVM's va_copy
  • we do still have our own va_copy intrinsic that serves as a hook for const evaluation
  • va_copy has a fallback body, code generation backends should not provide their own implementation

I've changed the signature of the intrinsic to be more ergonomic, and made it safe because while the hook is used to detect UB, the intrinsic itself is completely safe.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot

This comment has been minimized.

@folkertdev
Copy link
Contributor Author

Right so I've now included the changes to va_end that are required for const evaluation. In short Drop now calls va_end which has a default implementation of being a no-op.

@workingjubilee workingjubilee changed the title c_variadic: use Clone instead of LLVM va_copy c_variadic: impl va_copy and va_end as Rust intrinsics Jan 14, 2026
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Yeah, I think the names should remain the same as they are now because those are the names most familiar to C programmers, and I think that (soon-to-be-former, hopefully) C programmers poking at the source will want to see them instead of some offbeat va_list_clone call.

I do understand the implication that it is a "false friend", because someone might assume it is LLVM's impl. However, here we do document it is not, and people who learn how Rust's intrinsics work will know that it is at most a delegation to LLVM's intrinsics (and how to find what they do). They will first see the Rust source, anyways!

And in a sense, I think they aren't false friends: they are faithful implementations of va_copy and va_end... in Rust, a language which isn't C, but nonetheless implements some C semantics by assuming the C implementation can be made-compatible with Rust semantics.

We could name them rust_va_copy and rust_va_end if you really want to discourage people from assuming they are merely LLVM calls. I also do appreciate that you have spent a while with your head "in LLVM" and you have to make such a distinction between @llvm.va_copy and our code anyways. I think LLVM makes the right choice here... even if the intrinsic, in practice, isn't actually what clang uses 99.9% of the time (in terms of compiler invocations, anyways? maybe it's used for more targets in absolute count than I think it is, but then again LLVM doesn't have such a highly reified notion of target tuples like we do...).

View changes since this review

@workingjubilee
Copy link
Member

workingjubilee commented Jan 14, 2026

...I also am aware that copy might be confusing to Rust-first programmers who are overcalibrating on it being a Copy, and to them I say: tough. C is allowed to use words differently, so I think we wind up with this cross-language confusion anyways, because if we rename away from va_copy, people may go "where's the va_copy making this new va_list a valid thing?" We keep things most coherent, I feel, by focusing on how our impl reflects the standard's requirements.

@folkertdev
Copy link
Contributor Author

Makes sense, and yeah there are no obviously better names, so maybe consistency is more valuable. Let's see what Ralf thinks.

@RalfJung
Copy link
Member

Given that this is an internal API I'd rather have it use Rust-y terms than follow C terminology. We use the LLVM names for many of the intrinsics and this can be quite confusing. We can always explain the relation to va_copy in the docs.

But given that this is an internal API I also do not care very strongly.

@folkertdev
Copy link
Contributor Author

Because this is an internal name we can change it at any time. I'd propose to keep the names for now, actually implement const evaluation, and then re-evaluate if there is an obviously better name for what we use the intrinsic for.

@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2026

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@folkertdev
Copy link
Contributor Author

I updated a bunch of the comments

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2026
@rust-log-analyzer

This comment has been minimized.

///
/// See the [LLVM source] and [GCC header] for more details.
///
/// `va_copy` is `memcpy`: <https://github.com/llvm/llvm-project/blob/5aee01a3df011e660f26660bc30a8c94a1651d8e/llvm/lib/Target/PowerPC/PPCISelLowering.h#L713>
Copy link
Member

@RalfJung RalfJung Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also just a header, not the implementation? (same for some of the other ones)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah now I understand what happened: i copied these links from a github search result page. I manually looked them all up now, so all links should be correct.

@folkertdev folkertdev force-pushed the va-list-copy branch 2 times, most recently from 94ce7b4 to 6011fa6 Compare January 19, 2026 20:55
@RalfJung
Copy link
Member

Sounds great, thanks! r=workingjubilee,RalfJung with the last commit squashed away.

@bors delegate+

@rust-bors
Copy link
Contributor

rust-bors bot commented Jan 20, 2026

✌️ @folkertdev, you can now approve this pull request!

If @RalfJung told you to "r=me" after making some further change, then please make that change and post @bors r=RalfJung.

@folkertdev
Copy link
Contributor Author

@bors r=workingjubilee,RalfJung

@rust-bors
Copy link
Contributor

rust-bors bot commented Jan 20, 2026

📌 Commit 922057c has been approved by workingjubilee,RalfJung

It is now in the queue for this repository.

@rust-bors rust-bors bot added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. F-c_variadic `#![feature(c_variadic)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants